Skip to content

Fix phpstan/phpstan#10862: Invalid array key for constant array append when negative keys are used since PHP 8.3#5412

Open
phpstan-bot wants to merge 16 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-znh9m95
Open

Fix phpstan/phpstan#10862: Invalid array key for constant array append when negative keys are used since PHP 8.3#5412
phpstan-bot wants to merge 16 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-znh9m95

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

Summary

PHP 8.3 completed the negative array index RFC (originally from PHP 8.0) by fixing $a[] = value to use max_existing_key + 1 even when all keys are negative. PHPStan always computed the next auto-index as max(0, max_key + 1), producing incorrect types like array{-4: 1, 0: 2} instead of array{-4: 1, -3: 2} on PHP 8.3+.

Changes

  • src/Type/Constant/ConstantArrayTypeBuilder.php: Extended the auto-index update condition to also trigger for negative keys on PHP 8.3+ when no positive key has been set yet (i.e., when $max === 0, the initial default)
  • Added PhpVersionStaticAccessor import to access PHP version info

Root cause

In ConstantArrayTypeBuilder::setOffsetValueType(), the condition $offsetValue >= $max controlled whether a key updates the next auto-index. Since nextAutoIndexes starts at [0], negative keys (e.g., -4) never satisfied $offsetValue >= 0, so the auto-index stayed at 0. On PHP 8.3+, negative keys should update the auto-index because PHP's internal behavior changed to track negative keys for auto-increment.

Test

Added tests/PHPStan/Analyser/nsrt/bug-10862.php with four scenarios:

  • Simple negative key append ($a[-4] = 1; $a[] = 2 → keys -4, -3)
  • Negative key where +1 equals 0 ($a[-1] = 'x'; $a[] = 'y' → keys -1, 0)
  • Multiple negative keys ($a[-10] = 'a'; $a[-5] = 'b'; $a[] = 'c' → keys -10, -5, -4)
  • Mixed negative and positive keys ($a[-3] = 'a'; $a[5] = 'b'; $a[] = 'c' → keys -3, 5, 6)

Fixes phpstan/phpstan#10862

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a single nsrt file use two

bug-10862-php82.php‎
bug-10862-php83.php‎

One for php <=8.2 behavior and one for php >= 8.3.

if ($offsetValue >= $max) {
if (
$offsetValue >= $max
|| ($offsetValue < 0 && $max === 0 && PhpVersionStaticAccessor::getInstance()->getVersionId() >= 80300)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of

PhpVersionStaticAccessor::getInstance()->getVersionId() >= 80300

check,
introduce a dedicated method inside PHPVersion

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Here's what I changed to address the review:

  1. Added PhpVersion::updatesAutoIncrementKeyForNegativeValues() — a dedicated method in src/Php/PhpVersion.php that returns true for PHP >= 8.3, replacing the inline getVersionId() >= 80300 check in ConstantArrayTypeBuilder.

  2. Split the test file — replaced bug-10862.php with two version-specific files:

    • bug-10862-php82.php (// lint <= 8.2) — asserts the old behavior where negative keys don't affect auto-index (next key is 0)
    • bug-10862-php83.php (// lint >= 8.3) — asserts the new behavior where negative keys update auto-index

Both make phpstan and make tests pass cleanly.

@VincentLanglet VincentLanglet self-assigned this Apr 6, 2026
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Everything is already in good shape. The remote branch already has both review comments addressed:

  1. PhpVersion method: Added updatesAutoIncrementKeyForNegativeValues() method in src/Php/PhpVersion.php instead of inline version check
  2. Split test files:
    • bug-10862-php82.php with // lint <= 8.2 — tests old behavior (auto-index stays at 0 for negative keys)
    • bug-10862-php83.php with // lint >= 8.3 — tests new behavior (auto-index follows negative keys)

Both static analysis and tests pass. The commits are already pushed to the remote.

@VincentLanglet VincentLanglet requested a review from staabm April 6, 2026 18:14
Comment on lines +214 to +215
$offsetValue >= $max
|| ($offsetValue < 0 && $max === 0 && PhpVersionStaticAccessor::getInstance()->updatesAutoIncrementKeyForNegativeValues())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract this condition into a private method, to make the code more readable

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already handled — tests passed as well. Everything is done: the review comment has been addressed, tests and static analysis pass, and the commit is pushed.

@VincentLanglet VincentLanglet requested a review from staabm April 7, 2026 08:22
VincentLanglet and others added 6 commits April 7, 2026 11:14
- PHP 8.3 fixed an incomplete RFC implementation where $a[] after negative keys should use max_key+1, not 0
- Updated ConstantArrayTypeBuilder to account for this when computing nextAutoIndexes
- Added regression test in tests/PHPStan/Analyser/nsrt/bug-10862.php

Closes phpstan/phpstan#10862
Replace inline version check with a dedicated PhpVersion method
for the PHP 8.3+ negative array auto-index behavior.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Separate version-specific assertions into two test files with
lint version constraints instead of a single file.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…array literal behavior

PHP 8.0 changed array literals to update the auto-increment key for
negative values, while imperative assignment only got this fix in PHP 8.3.
Extract the condition into shouldUpdateAutoIndexForOffset() for readability
and add isLiteralArray flag to ConstantArrayTypeBuilder to distinguish the
two cases.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split tests into four files covering all three PHP version behaviors:
- php74: pre-PHP 8.0, negative keys never affect auto-index
- php80: PHP 8.0+, array literals with negative keys update auto-index
- php82: PHP <= 8.2, imperative assignment does not update for negative keys
- php83: PHP 8.3+, negative keys always update auto-index

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@staabm staabm force-pushed the create-pull-request/patch-znh9m95 branch from ef47efc to 9ac8db3 Compare April 7, 2026 09:14

private bool $oversized = false;

private bool $isLiteralArray = false;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the "isLiteralArray" naming matches reality. we need to find a better name

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed. Here's what I did to address staabm's review:

Renamed isLiteralArrayisArrayExpression across all three files:

  • ConstantArrayTypeBuilder::$isLiteralArray$isArrayExpression
  • ConstantArrayTypeBuilder::setLiteralArray()setArrayExpression()
  • PhpVersion::updatesAutoIncrementKeyForNegativeValuesInArrayLiteral()updatesAutoIncrementKeyForNegativeValuesInArrayExpression()
  • InitializerExprTypeResolver call site updated accordingly

The term "array expression" better matches PHP parser terminology (Expr\Array_) and PHPStan's own naming conventions.

Regarding staabm's second concern (whether this is needed beyond InitializerExprTypeResolver::getArrayType()): The flag is correctly scoped. It only needs to be set when processing Expr\Array_ nodes because that's the only context where PHP 8.0-8.2 changed the negative auto-index behavior. When createFromConstantArray() is used (e.g., for $a[] = 2 after $a = [-4 => 1]), the nextAutoIndexes are already correctly inherited from the source ConstantArrayType — the flag was already applied when the array expression was first built. Other createEmpty() call sites build arrays from PHPDoc types, function return types, etc., where PHP runtime auto-index semantics don't apply.

}

$arrayBuilder = ConstantArrayTypeBuilder::createEmpty();
$arrayBuilder->setLiteralArray();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it really make sense only in this scenario? don't we need this behaviour every time a array-shape with known/constant keys is involved?

The flag marks builders processing Expr\Array_ nodes (PHP array
expression syntax), which have different auto-index semantics for
negative keys in PHP 8.0-8.2 vs imperative assignment. The term
"array expression" better matches the PHP parser terminology.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the comment php/php-src#11154

The bug fixed in PHP 8.3 was on empty array.
So, it seems like

  • Prior to PHP8, it doesnt update auto increment
  • From PHP8 to PHP 8.2 it does update auto imprecrement if the array was initialized as non empty.
  • After PHP 8.3 it does update every time

So Rather than ArrayLiteral or ArrayExpression, is should express the fact the array is initialized with values [$foo => $bar] or not [].

…array initialization

The distinction is whether the array was initialized with values (e.g. [-4 => 1])
vs initialized empty ([]). This matches the actual PHP behavior:
- Pre PHP 8.0: negative keys never update auto-increment
- PHP 8.0-8.2: updates only when array initialized non-empty
- PHP 8.3+: always updates

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Done. Here's what changed:

  • isArrayExpressioninitializedNonEmpty — property, setter (setInitializedNonEmpty()), and all references renamed
  • updatesAutoIncrementKeyForNegativeValuesInArrayExpression()updatesAutoIncrementKeyForNegativeValuesInNonEmptyInitializer() — PhpVersion method renamed
  • Guard added: setInitializedNonEmpty() is now only called when count($expr->items) > 0, so empty array expressions ([]) don't get flagged

The naming now correctly expresses the PHP behavior: the flag indicates the array was initialized with values ([-4 => 1]) rather than starting empty ([]), which is what triggers the PHP 8.0-8.2 auto-increment behavior for negative keys.

@VincentLanglet VincentLanglet requested a review from staabm April 8, 2026 14:22

$arrayBuilder = ConstantArrayTypeBuilder::createEmpty();
if (count($expr->items) > 0) {
$arrayBuilder->setInitializedNonEmpty();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than an extra method, what about an optional param in createEmpty ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Replaced setInitializedNonEmpty() with an optional bool $initializedNonEmpty = false parameter on createEmpty(), and updated the call site in InitializerExprTypeResolver to pass count($expr->items) > 0 directly. Both make phpstan and make tests pass.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if createEmpty always set initializedNonEmpty = true ?

I feel like

  • either createEmpty is used and getType is called so initializedNonEmpty didn't change things
  • either createEmpty is used to setOffset and then it means it's initializedNonEmpty.

Maybe the only place when it's initializedEmpty is when it's called with createFromConstantArray and an Empty one ?

createEmpty() is only used to build arrays from scratch (array
expressions, type operations, function return types). The imperative
case ($a = []; $a[-4] = 1;) goes through createFromConstantArray(),
which correctly defaults initializedNonEmpty to false. So createEmpty()
can always assume the array is initialized non-empty — the parameter
is unnecessary.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

The background test run also completed successfully (exit code 0), confirming the change is clean.

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than setting

$self->initializedNonEmpty = true;

in createEmptyArray,

use an opposite property like initializedEmpty or better name and set it in createFromConstantArray

…nstantArray

Instead of setting initializedNonEmpty=true in createEmpty(), use the
inverse property initializedEmpty and set it in createFromConstantArray()
when the source array has no keys. This is cleaner because createEmpty()
is always used to build arrays that will have items added, while the only
truly "initialized empty" case is createFromConstantArray with an empty
ConstantArrayType (representing $a = [] imperative assignment).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Done. The change replaces initializedNonEmpty (set in createEmpty()) with initializedEmpty (set in createFromConstantArray() when the source array is empty). The logic is inverted accordingly in shouldUpdateAutoIndex():

  • createEmpty()initializedEmpty = false (default) — used for building arrays that will have items added (literals, function return types, etc.)
  • createFromConstantArray(emptyArray)initializedEmpty = true — represents $a = [] imperative assignment where PHP 8.0-8.2 doesn't update auto-index for negative keys
  • createFromConstantArray(nonEmptyArray)initializedEmpty = false (default) — auto-indexes already correctly inherited

Both make phpstan and make tests pass cleanly.

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest commit produce this error on PHP 8.0/8.1

1) PHPStan\Analyser\NodeScopeResolverTest::testFile with data set "tests/PHPStan/Analyser/nsrt/bug-10862-php82.php" ('/home/runner/work/phpstan-src...82.php')
Failed assertions in /home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/nsrt/bug-10862-php82.php:

Line 33:
Expected: array{-10: 'a', -5: 'b', 0: 'c'}
Actual:   array{-10: 'a', -5: 'b', -4: 'c'}

…round-trips

When processing `$a = []; $a[-10] = 'a'; $a[-5] = 'b';`, the second
assignment creates a new builder via createFromConstantArray with a
non-empty array, losing the initializedEmpty flag. This caused
shouldUpdateAutoIndex to incorrectly return true on PHP 8.0-8.2.

Fix: in createFromConstantArray, also infer initializedEmpty when the
source array has negative int keys but max(nextAutoIndexes) === 0,
since a non-empty initializer on PHP 8.0+ would have already bumped
the auto-index.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Pushed. The fix addresses the issue where initializedEmpty wasn't propagating through createFromConstantArray round-trips.

Root cause: When processing $a = []; $a[-10] = 'a'; $a[-5] = 'b';, the second assignment creates a new builder from array{-10: 'a'} (non-empty), so initializedEmpty defaulted to false. This made shouldUpdateAutoIndex incorrectly return true on PHP 8.0-8.2, producing -4 instead of 0 as the next auto-index.

Fix: In createFromConstantArray, also set initializedEmpty = true when the source array has negative int keys but max(nextAutoIndexes) === 0. This is a reliable indicator that the array was originally empty-initialized — because a non-empty initializer on PHP 8.0+ would have already bumped the auto-index past 0.

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test which fail if the call $builder->initializedEmpty** = true; was made without condition inside createFromConstantArray

…dition

This test would fail on PHP 8.0-8.2 if initializedEmpty was set
unconditionally in createFromConstantArray. With only string keys,
max(nextAutoIndexes) is 0 but there are no negative int keys, so
initializedEmpty should stay false, allowing negative keys to update
the auto-index.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

The background test run also completed successfully (exit code 0), confirming the change is clean.

The initializedEmpty flag is only consulted in shouldUpdateAutoIndex on
PHP 8.0-8.2. Computing it on other versions is both semantically
incorrect (on PHP < 8.0, the nextAutoIndexes heuristic can't distinguish
empty-init from literal-init) and wasteful (the flag is never used).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the check

elseif (max($startArrayType->getNextAutoIndexes()) === 0) {
				foreach ($startArrayType->getKeyTypes() as $keyType) {
					if ($keyType instanceof ConstantIntegerType && $keyType->getValue() < 0) {
						$builder->initializedEmpty = true;
						break;
					}
				}
			}

shouldn't exist and the issue is the fact that the nextAutoIndexes is wrong.

@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Pushed. Here's what changed:

Moved initializedEmpty from a heuristically-reconstructed builder flag to a persisted ConstantArrayType property.

The elseif block that scanned keys and nextAutoIndexes to guess whether the array was empty-initialized is eliminated. Instead:

  1. ConstantArrayType now stores initializedEmpty as a constructor parameter (optional, default false for BC), with a getter and automatic propagation through recreate()
  2. ConstantArrayTypeBuilder::getArray() passes the flag when creating ConstantArrayType
  3. createFromConstantArray() reads it directly from the type: $startArrayType->isInitializedEmpty() || count(keys) === 0

The flag naturally survives setOffsetValueType → createFromConstantArray → builder → getArray round-trips without any key-scanning heuristic.

@VincentLanglet VincentLanglet force-pushed the create-pull-request/patch-znh9m95 branch from eefb449 to 4a7fe14 Compare April 8, 2026 19:24
@VincentLanglet
Copy link
Copy Markdown
Contributor

I'm not fully satisfied by the state of the PR but I'm not fully sure why and I'm lacking of ideas.
Any suggestions @staabm ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants